Skip to content

[JENKINS-63355] Prevent NPEs when library name is not provided #101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

[JENKINS-63355] Prevent NPEs when library name is not provided #101

wants to merge 10 commits into from

Conversation

@Dohbedoh
Copy link

@Dohbedoh Dohbedoh commented Dec 18, 2023 •
edited
Loading

JENKINS-63355 Prevent NPEs in cases where the library name is not provided (if a library without name is accessible - in the context of a pipeline - then the library step would fail with a NPE, regardless of the library it tries to find)

Testing done

  • Start a controller
  • Add a Global Pipeline Library pointing to a repo with a hello world step.
  • Create a Folder, use defaults
  • Create a hello world pipeline that uses the above library via the library step. e.g. library "test-steps"; helloWorld()
  • Run the pipeline to confirm that it succeeds
  • Configure Folder
    • Find "Pipeline Libraries" section and click "Add"
    • Ignore the error about the library name and click "Save"
  • Run the pipeline again
  • Make sure that the pipeline succeeds
### Submitter checklist
- [x] Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!
- [x] Ensure that the pull request title represents the desired changelog entry
- [x] Please describe what you did
- [x] Link to relevant issues in GitHub or Jira
- [x] Link to relevant pull requests, esp. upstream and downstream changes
- [x] Ensure you have provided tests - that demonstrates feature works or fixes the issue

@Dohbedoh Dohbedoh requested a review from a team as a code owner December 18, 2023 13:04
@jglick jglick added the bug Something isn't working label Dec 18, 2023
jglick
jglick reviewed Dec 20, 2023
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what happens if a library with a null name was saved before this change? @LocalData

src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryConfiguration.java Outdated
for (LibraryResolver resolver : ExtensionList.lookup(LibraryResolver.class)) {
for (LibraryConfiguration config : resolver.fromConfiguration(Stapler.getCurrentRequest())) {
if (config.getName().equals(name)) {
if (name.equals(config.getName())) {
Copy link
Member

@jglick jglick Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be reverted

src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryStep.java
for (LibraryResolver resolver : ExtensionList.lookup(LibraryResolver.class)) {
for (LibraryConfiguration cfg : resolver.forJob(run.getParent(), Collections.singletonMap(name, version))) {
if (cfg.getName().equals(name)) {
if (name.equals(cfg.getName())) {
Copy link
Member

@jglick jglick Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@Dohbedoh
Copy link
Author

Dohbedoh commented May 14, 2024

So what happens if a library with a null name was saved before this change?

Actually still fails. Need to rework this. We need to skip those libraries. But probably also tell the user about it.

@jglick jglick marked this pull request as draft May 14, 2024 12:50
@Dohbedoh
Copy link
Author

Dohbedoh commented May 21, 2025

The IAE in the constructor would interrupt the resolution in:

If only we could pass a TaskListener to the LibraryResolver#forJob, we would be able to skip null named library in the for loop during resolution while logging through the task listener (in the build log). But touching LibraryResolver would require to updated some dependent plugin that implements this extension point. Otherwise, we could log a warning through the class logger (Jenkins log)...
Another solution is to not throw an IAE in the constructor, let LibraryResolvers find those libraries, and handle the skip in the callers (LibraryAdder / LibraryStep).

WDYT ?

@jglick
Copy link
Member

jglick commented May 27, 2025

we could log a warning through the class logger (Jenkins log)

Seems enough?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jglick jglick jglick left review comments

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Dohbedoh @jglick